Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat -- table owner categories #2260

Merged
merged 10 commits into from
Sep 25, 2024
Merged

feat -- table owner categories #2260

merged 10 commits into from
Sep 25, 2024

Conversation

B-T-D
Copy link
Contributor

@B-T-D B-T-D commented Aug 14, 2024

Description

This PR adds config support for differentiating multiple categories of table owners in the Amundsen UI. For example, if an Amundsen instance wants to differentiate human users who are responsible for remediating operational issues with a table from non-human team or service emails.

To implement this, we propose supporting open-ended owner categories identified by a name and a definition text, to be configured in non-open-source code. The relevant React components are updated to render differently when owner categories are provided, while preserving current behavior for Amundsen instances that don't want to display different owner types differently in the UI.

  • Add OwnerCategory interface
  • Update User interface to support passing other_key_values field that already exists on the backend's User model
  • Update OwnerEditor component to render table owners grouped by category, when categories are configured

Example of owners rendered by category (where there's no owner of a configured category for the table, we still show the category name and definition, e.g. no "operational" owners here):
owner categories and definition info text

Motivation and Context

At Lyft, we'd like to be able to differentiate human-user table owners--who could actually be contacted to address data quality problems--from non-human owners.

How Has This Been Tested?

Existing unit tests and linting pass. I locally tested using the new configs to have the React app render owners by category.

CheckList

  • PR title addresses the issue accurately and concisely
  • Updates Documentation and Docstrings
  • Adds tests
  • Adds instrumentation (logs, or UI events)

@boring-cyborg boring-cyborg bot added the area:frontend From the Frontend folder label Aug 14, 2024
@B-T-D B-T-D changed the title owner category interface feat -- table owner categories Aug 28, 2024
Copy link
Member

@Golodhros Golodhros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

<ul className="component-list">
{section ? (
<div>
<span>{section.label}</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add className="title-3" to this so it looks more like a title?


const ownerList = hasItems ? (
return (
<ul className="component-list">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a css class to the <ul> here that sets margin-bottom: $spacer-1 (8px) but then sets it to 0 for the last-child? the larger sections have their own spacing so we want to skip it for the last item

</div>
);
}
renderOwnersSection = (section: OwnerCategory | null) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might want to consider doing some indication if there are no owners in a certain category. right now those titles look like floating text but isn't fully clear that it is an empty list

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@kristenarmes kristenarmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for making the updates!

@B-T-D B-T-D merged commit f6949e7 into main Sep 25, 2024
11 checks passed
@B-T-D B-T-D deleted the btd-table-owner-categories branch September 25, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:frontend From the Frontend folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants